Add CLI on top of config keys refactor - #6082
paullinator wants to merge 32 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
67406a5 to
35eeb43
Compare
a8819b9 to
60adf1e
Compare
7d3a2ba to
c0c5f13
Compare
0574066 to
4bb295e
Compare
4bb295e to
70e9045
Compare
Exchange rates could not be fetched outside the app: the module read its server list and its error reporter at load time, both of which come from React Native. The query logic now takes what it needs through `configureExchangeRates`, and `exchangeRatesGui` supplies the GUI's Airship reporter at app start. A Node caller supplies its own, so the same rate lookup answers the same way in both places.
`initLocale` reaches for `react-native-localize`, so nothing outside the app could ask which locale to use. The decision itself is pure: read a tag from argv, config or the environment, normalize it, and pick a language table. `nodeLocale.ts` holds that decision with no React Native imports, and feeds the same `applyLocale` that the GUI's device lookup already calls, so the GUI and any Node caller resolve a locale the same way rather than approximately the same way. Precedence is explicit and tested: an explicit tag, then config, then `EDGE_CLI_LOCALE`, then `LC_ALL` / `LC_MESSAGES` / `LANG`, then `Intl`, then `en-US`. `es_MX.UTF-8@euro` and `C` both resolve, which is what the POSIX forms actually look like. `env` is typed as the variables it reads rather than `NodeJS.ProcessEnv`, which in this repo demands `NODE_ENV` and would make every caller invent one.
`CategoriesActions.ts` held five hundred lines deciding what a transaction should be called: the category, the payee, the direction, and the label for each action type. All of it is a pure function of the transaction, the wallet and the account, but it sat behind Redux imports, so nothing outside the app could ask the same question and get the same answer. `src/util/txDisplay/` holds that logic now — `displayInfo` for the derivation, `category` for the category strings, `txActionLabels` for the action names, and `currencyCodes` for the ticker lookups. The call sites move with the helpers: `CategoryModal`, `TransactionDetailsScene` and `TransactionListRow` import `splitCategory`, `joinCategory`, `getTxActionDisplayInfo` and `Category` from `util/txDisplay` instead of from `CategoriesActions`. Every one of those is an import path, so no scene's behaviour changes. The point is that two callers cannot drift. A transaction rendered in a list, exported to CSV, or printed by a script now describes itself identically, because it is the same code deciding.
Three small pieces of GUI state that any caller reading transactions needs, and none of which had a reason to be Redux-only. `exchangeDenom` picks the denomination a currency or token reports amounts in. `DenominationSelectors` keeps its selector shape and calls it, so the two cannot disagree about what a multiplier is. `spamThreshold` decides which incoming transactions are dust worth hiding. The GUI applies it to every list; a caller that reads the same wallet and does not apply it sees a different set of transactions, which is the sort of difference that looks like a bug in whichever one you did not write. `localAccountSettings` reads the device-local settings file that holds the spam filter toggle, and `LocalSettingsActions` reads through it rather than duplicating the format. The threshold needs a rate, and asks for it at the current hour rather than the current millisecond, so repeated listings share one cache entry instead of missing on every call. It is a different source from the GUI's, which reads live rates from Redux, and a lookup that fails yields no filtering — stated in the module, because the two can legitimately disagree. `syncedSettingsFile` holds the one definition of the synced `Settings.json` that Node-safe code reads. The GUI's own cleaner sits behind an Airship import and cannot be loaded here, so this is a two-field view of the same file with the same defaults, and a test asserts those defaults still match the GUI's.
`TransactionExportActions.tsx` was a five-hundred-line thunk that did four separable jobs: fill in historical fiat values, render CSV, render QBO, and render the Bitwave format. Only the last step needed React Native, and only for writing the file. `fillTxsFiat` asks the rates server what each transaction was worth on the day it happened, which is the part that makes an export more than a dump of native amounts. `txExport/format` renders the three formats. `exportTxInfo` holds the Bitwave account mapping the exporter needs. The thunk keeps the file-writing and the share sheet, and calls the same renderers. `TransactionsExportScene` follows it. A test now covers `fillTxsFiat` against a partial wallet, which is all it reads. The formats matter here: an export that a person reconciles against their books has to be byte-identical whichever tool produced it, and the only way to be sure of that is for one renderer to produce both.
`SendScene2` saved a sent transaction and then attached its metadata, its category, its notes and any swap details in a sequence that had to happen in a particular order and had grown inline in the scene. A second caller that saved a transaction and got the order wrong would produce a transaction that looks right until someone exports it. `txTagging/apply` holds that sequence. `SendScene2` calls it and loses two dozen lines. The scene was the only definition of what a correctly tagged transaction is, and now it is not the only caller that can produce one. It re-applies only the fields the caller actually supplied. Under `EdgeMetadataChange` an empty string is a value rather than "leave unchanged", so passing all three through would let a caller who set one field erase the other two that core derived. The trigger is any non-empty name, notes or category, which is wider than the scene's old `payeeName != null` check and preserves a category that would otherwise be lost; callers pass the metadata they were given, never computed display metadata.
A long-lived engine daemon owns the `EdgeContext` and answers a JSON
REST API over a Unix socket; the `edge-cli` binary is a thin one-shot
client that spawns the engine on demand and keeps a session id in
`session.json` so commands chain. `docs/EDGE_CLI.md` describes that
architecture and deliberately documents no endpoints — the reference is
generated.
The point of this commit is the declaration format, so it carries
thirteen calls. Each is one `route({…})`: the core call it fronts, the
HTTP method and path, how it appears on the command line, cleaners for
the query, body and response, and its error codes. The prose lives
inside the declaration, beside the field it describes, and the JSDoc
above carries what belongs to the call as a whole.
Nothing is written twice. The command line, the help text, the OpenAPI
document and the HTML reference are all derived from these declarations,
and the derived artifacts are committed so a fresh clone needs no build
step. Five gates run in the pre-commit hook and reject the ways they
could drift apart: a route with no command, a handler reading a field
its cleaner would strip, a request parameter the core call does not
have, a generated file that is stale, and a command no test exercises.
The thirteen cover the shapes worth reviewing:
- no arguments, engine-local — `engine-status`, `engine-config`
- no arguments, reaching core — `local-users`, `fetch-login-messages`
- one named argument — `username-available`
- a body, and the session it establishes — `create-account`,
`login-with-password`, `logout`
- a positional path parameter — `object-get`, `object-delete`
- a held-open stream — `subscribe`
Path parameters are base58 identifiers and nothing else, because base64
wallet ids and free-text usernames contain `/` and cannot survive a URL
unescaped. Everything else is a named argument. A positional is declared
once as an ordinary field and the path is derived from it, so the two
cannot disagree.
`--fake` serves an in-process `makeFakeEdgeWorld`, which is what lets
the CLI tests run in a hook with no network, no server and no API key.
Core values with methods on them cannot cross JSON, so the engine keeps
them and hands back a handle: a staged transaction, a pending login, a
swap quote, a lobby. A handle carries its own TTL and is released when
the caller finishes with it, and a call that consumes one — approving a
swap — marks it in flight first, so a client that retries after its own
socket timeout is refused with `OBJECT_IN_USE` rather than spending
twice. A call that keeps its handle holds it the same way, so a
broadcast that outlives the TTL still returns its txid instead of
expiring between the send and the reply. Reading a handle returns a
projection, never the live object: serializing an `EdgeAccount` would
walk its `otpKey` and `recoveryKey` getters, and a swap quote reaches
both wallets and every token they know.
Responses are validated against the same cleaners that document them.
`checkResponse` runs each one and discards the cleaned value, since a
cleaner strips unknown keys and returning it would delete fields the
engine means to send; `EDGE_CLI_CHECK_RESPONSES` decides whether a
mismatch warns, fails the request, or is skipped. Drift shows up in the
log rather than reaching a caller unnoticed.
One engine serves one profile, and it claims the profile by creating its
run file exclusively before opening the data directory, so two cold
invocations cannot hold two `EdgeContext`s on one set of repos or unlink
each other's socket. The idle timer counts in-flight requests as well as
sessions and subscribers, so a cold login cannot be shut down underneath
itself. Everything the engine reads from disk — its run file, the
client's session file, the account's synced settings — goes through a
cleaner, and the bearer tokens in an OTP challenge are masked on their
way to a terminal while staying in the REST body the commands read them
from.
Account and session management, credentials, 2FA and vouchers, the data
store, keys and wallets, tokens, URIs, transactions and their export,
the staged spend path, swaps, exchange rates, and the `$internalStuff`
admin calls — a hundred and four `route({…})` declarations, each
carrying its own cleaners, error list and prose.
The five documentation gates hold across every one of them: the surface
matches, each response field carries prose, each call either matches its
`edge-core-js` signature or records why it differs, and all but four run
offline against the fake world. `npm run docs:api:gates` reports the
counts; they are not repeated here, because a number in a commit message
goes stale the first time a field is added.
Four cannot be exercised in a hook and say so: the two rates calls, swap
quotes and payment-protocol requests each reach a third-party API that
the fake world does not intercept. No suite covers them:
`test:cli:network` runs the one-shot, CAPTCHA and edge-login suites, and
none of the three names any of the four. The coverage gate records that
as the reason it excuses them.
Where the API departs from `edge-core-js` it is recorded in `coreExtra`
with the reason — a wallet object that cannot cross HTTP as anything but
an id, the `to`/`amount` shorthand that expands into `spendTargets`,
engine-side paging and export on `get-transactions`. Anything not listed
there fails the build.
Where a value cannot be derived safely the call refuses rather than
guesses. `rates-usd-to-native` requires its `multiplier`, because this
route has no logged-in account to read a denomination from and an
assumed one returns a `nativeAmount` wrong by orders of magnitude.
`sign-bytes` rejects malformed base64 instead of signing whatever
decoded. Handles record the resolved `wallet.id`, so a wallet id and a
unique prefix of it name the same wallet on every step of a staged
spend.
The CLI is built from this repository but is not this repository: rollup inlines every module it reaches under `src/`, so a published package is the two bundles, the native HMAC addon, the CLI document as its README and `LICENSE`. Nothing about it needs the CLI to move to a workspace or a submodule, and the app's own `package.json` stays `private: true` — what is published is a separate manifest assembled in a temporary directory, so the app itself cannot reach npm by accident. `src/cli/npmMeta.ts` holds the decisions: the scoped name, the bin name, the licence, and the per-platform native packages, which become `optionalDependencies` once they exist. The version is not among them. The CLI ships in lockstep with the app, so the manifest takes it from the app's `package.json`: there is no second number to bump, and a published CLI says which app release it corresponds to. Lockstep costs one thing worth knowing before a release — npm will not replace an existing version, so a CLI-only fix goes out on the next app version bump rather than on its own. The dependency list is derived, not written down. `rollup.config.cli.mjs` externalises every key of the app's `dependencies` — its whole runtime set — so the bundles leave all of them as bare `require`s while needing fifteen, and anything the app does not declare is inlined instead. `buildCliManifest.ts` walks the module graph from both entry points, keeps the bare specifiers the app declares as dependencies, drops builtins and type-only imports, and treats the rest as bundled. Checked against the bundles' own `require` calls: fifteen declared, fifteen required, none missing and none spare. Hand-maintaining that list fails as an `npm install` that succeeds and a CLI that cannot resolve a module on first run, so `cli:manifest:check` gates it in `verify` and in CI — where, unlike the five documentation gates, `npm run prepare` does not regenerate it first. `publishCli.ts` builds, stages and publishes. With `edgeKey.json` it runs `build:cli:all`, so a build server needs that one file to produce a CLI with full native signing; without it the addon cannot be built, so publishing takes an explicit `--allow-unsigned` and the staged README says the build cannot sign. A dirty tree is refused, since the registry copy could not then be re-derived from any commit. `--dry-run` packs without publishing and `--out` stages for inspection. Verified end to end: the staged tarball is 924 kB over six files, and installed into an empty project the client answers `--help` and the engine boots and serves `engine-status`. Two things the exercise surfaced, neither fixed here. `uuid` is imported by `src/util/utils.ts` and declared nowhere, resolved only because other packages happen to depend on it; rollup inlines it, so the published CLI is unaffected, but the build rests on a transitive resolution. And installing the package costs 2.3 GB across 69,756 files, of which about 1.3 GB is React Native mobile binaries — iOS simulator slices and Android libraries for the privacy coins — reached through `edge-currency-accountbased`, which also brings `react-native` itself. A Node CLI can load none of it.
3.review-errors.2, with 3.engine-routes.1 and 3.engine-infra.2. Five request-position amount fields were declared `asString` and handed to biggystring, which throws a plain `Error` that `toErrorBody` has no arm for — so `spend --native-amount=abc` and `get-transactions --spam-threshold=abc` answered 500 on routes declaring 400. A fractional value was worse than an error: the UTXO plugin sums fees with biggystring but builds the output with `parseInt`, so "1.5" funded the fee math at 1.5 and paid out 1, under a field documented as the chain's smallest unit. `asIntegerString` already existed for exactly this and is now exported and used by `asSpendTarget`, the spend shorthand's `nativeAmount` and `amount`, `swap-quote`, `encode-uri` and `spamThreshold`, where zero stays legal. Response-position amounts keep `asString`: a transaction's `nativeAmount` is negative for a send, and core's own values are already valid. `asEdgeTxAction` dispatched through a plain object literal, so `actionType: "toString"` resolved `Object.prototype.toString` — not null, so the unknown-actionType guard was skipped and a string was returned as an `EdgeTxAction`; `"constructor"` handed back the unvalidated body. Both reached `wallet.saveTxAction`, where core dispatches `CURRENCY_WALLET_FILE_CHANGED` before its own uncleaner rejects. Guarded with the shared `hasOwn`, as `util/exchangeDenom.ts` already does. Seven offline checks cover the new rejections, including both prototype names.
3.review-state.6, and 3.review-state.7 with 3.harness-review.8.
`--save-export-prefs` was honoured on one branch of three: the
`mergeExportTxInfo` call sat inside `if (formats.includes('bitwave'))` and
again inside the explicit-account-id test, so
`--export-format=csv,qbo --save-export-prefs` answered `ok` and wrote
nothing. The caller asked for their format choice to be remembered and the
GUI export scene's switches were untouched. The write now runs once for
every format combination, still only when the flag is given — writing
unasked turned a one-off `--bitwave-account-id` into the user's saved id.
Passing an absent id is safe because `mergeExportTxInfo` reads each field
as `patch.x ?? prev?.x`, which is what keeps a saved bitwave id through a
csv-only save.
`limit`'s published description said "Defaults to 100" while
`DEFAULT_TX_LIMIT` is 99 — deliberately, so a page prices in one rates
request. The wrong number ships in `openapi.json`, the HTML reference and
`edge-cli help`, so a caller paging on the documented default walks
`offset` 0, 100, 200 and skips one transaction per page, which `total`
cannot reveal because it is the match count. The description cannot
interpolate the constant: `extractRoutes` reads it through the checker as a
string literal, and a template literal drops the description from the
reference altogether. A test holds the two in step instead.
3.engine-routes.3, 3.engine-routes.2, 3.harness-review.4 and 3.review-state.5 — four routes whose declared reach and real reach differed. The key-export calls resolved through `findWallet`, which searches `account.currencyWallets`: core builds that only from `activeWalletIds` and only for wallets whose api exists. So `all-keys` listed an archived wallet and `get-raw-private-key` answered `WALLET_NOT_FOUND` for the exact id it had just printed — on the disaster-recovery path a CLI key export is for. Core's `getRawPrivateKey`, `getDisplayPrivateKey`, `getRawPublicKey` and `listSplittableWalletTypes` all work off `allKeys`, so `findWalletId` resolves there, with the same prefix contract and the same errors. `change-wallet-states` validated nothing. Core treats an id it has never seen as new and writes a state file for it without complaint, so a typo or a prefix answered 204 while nothing changed, and left a bogus `Keys/<hash>.json` in the account repo to sync to every device. It was also the one wallet route that ignored the prefix contract `walletId` is documented with. Every key now resolves over `allKeys` first. `save-tx` was the only handle-advancing route setting neither `consuming` nor `hold`, so `OBJECT_IN_USE`, the sweeper's skip and a bulk release's bounded wait were all blind to it. A slow save overlapping a `broadcast-tx` for the same handle removed the record under the in-flight broadcast, which then answered `OBJECT_NOT_FOUND` after the funds had left. It runs under `consume` now, which also does the delete the handler did by hand. `object-get` and `object-delete` published a reach they lost: only `transaction` and `swap` are created with a `sessionId`, so `pendingLogin` and `lobby` can only answer `OBJECT_SESSION_MISMATCH` there. Both descriptions say so, and the two dead arms of `projectHandleValue` are explicit about being unreachable and point at `pendingSummary`, which is where a pending login is really projected — the reasoning worth keeping, since serialising one walks into `otpKey` and `recoveryKey` getters.
3.review-servers.4, and 3.review-state.3 with 3.harness-review.2.
`handleRequest` ran `idle.touch()` and `idle.beginRequest()` before
`checkTcpRequest`, so every rejected request pushed `idleShutdownAt` out by
a full `--idle-timeout`. Any other local process — the threat
`transportAuth.ts` names — could poll the port once a minute with no token
and keep the daemon resident indefinitely, holding its EdgeContext and
every plugin's polling open, which is the leak `idleShutdown.ts` exists to
bound. The comment three lines below already claimed this ordering ("a
caller that cannot authenticate learns nothing about this engine"); now it
is true. A rejection is also logged at `warn`, because it is the only sign
of a probe and nothing recorded it.
The auto-logout ticker skipped any session whose window was `0`, which made
that value a one-way latch: a session created while the setting said `0`
captured it and the ticker never looked again, so a user turning
auto-logout back on from their phone had no effect on a session the engine
was already holding, while `engine-sessions` kept reporting
`autoLogoutSeconds: 0` as though it were still their choice. The cost
argument behind the skip is sound — `isExpired` answers false for `0`
before it looks at a clock, and the read is a decrypt plus a parse with no
cache — so those sessions now re-read once a minute against the ticker's
fifteen seconds. A quarter of the reads, and a bound on the staleness of a
security control.
The test that asserted the skip now asserts the cadence, and a new one
turns auto-logout back on mid-session and expects the logout.
3.engine-infra.3, 3.harness-review.6 and the closing paragraph of 3.review-async.2 — one defect found three times. `cleanupStaleLock` required a listening socket as well as a live pid, and its own comment asserted that `sweepStaleProfiles` "skips the directory for the same reason". The sweep tested the pid alone. `process.kill(pid, 0)` succeeds for ever once the OS hands that pid to something else, so the sweep permanently skipped the directories it exists to clear: a SIGKILLed engine's `session.json` — a full-account bearer token, which `removeRunArtifacts` is specifically there to delete — survived in a profile nothing would revisit, because `testCliFake` derives its data directory from the pid and every run hashes fresh. That is the 428 directories with 235 session files the sweep was written for. Both callers now share one `isClaimLive`, so they cannot drift again: a claim is live when something answers on its socket, or when it is young enough to still be booting. `sweepStaleProfiles` becomes async for the probe, which is local to its one caller in the async startup. A new case covers the recycled pid directly — live pid, nothing listening, backdated past the boot grace — and asserts the session file goes.
3.review-performance.4 with 3.review-servers.6, and 3.review-code-quality.6. `apiClient` waited 15 s for a shutting-down engine to release its socket, under a comment claiming that was "bounded by the drain the engine itself allows, plus a margin" — the drain alone is 110 s, so the margin was negative by a factor of seven. A stop or a Ctrl-C landing while any request slower than 15 s was in flight, which the guide says `get-transactions` on a whole wallet is, left the socket bound and answering 503; the next command gave up, spawned a replacement, and that child failed `claimRunFile`'s `wx` against the dying engine's run file and printed "An engine is already running … Stop it first" — advice to do what the user had just done, and the exact sequence `isShuttingDown` exists to prevent. `shutdownTiming.ts` now holds the phases that stand between `shuttingDown = true` and the listeners closing — the request drain, a bulk handle release, a logout's wait — and the client's wait is their sum rather than a fourth number that contradicted them. The module has no logic and no imports, so the client reads the engine's figures without pulling the engine into its bundle, which a check on the built client confirms. `ApiClientOptions.host` and `.port` are gone. They were unreachable — every construction passes `socketPath` — and could not have worked: the engine's TCP listener requires `X-Edge-Token` and the client never sent one, so a caller reaching for them would have got a 401 with no clue why. The two transports were also chosen two different ways, so `openStream` would have kept using the socket while `request` switched. `--tcp` is forwarded to the engine for other local scripts, which is what the architecture listing now says. `testCliSubscribe` imports `TCP_TOKEN_HEADER` instead of spelling it five times — and spelling it by hand is what let a bad replacement of mine through until two checks that need a valid token caught it.
3.review-react.1 with 3.harness-review.5. `escapeOFXString`'s non-ASCII pattern had no `u` flag, so it matched one UTF-16 code unit at a time and `codePointAt(0)` saw half a surrogate pair: `Tip 🍕` came out as `Tip ��`. A lone surrogate is not a character in SGML, XML or OFX, so no importer can turn that back into the original — which is exactly what the docblock promises, for exactly the inputs the sentence above it lists: a payee or memo set by an `edgeProvider` dapp, by the CLI's `--metadata`, or typed into the GUI's notes field. It is also a regression against base for this input, which emitted the raw UTF-8 bytes. Every character in the existing charset case is BMP, which is why it passed. A second case covers an emoji and asserts the real code points rather than surrogate halves; it fails without the flag.
3.review-performance.2.
Rollup keeps an external module it cannot prove side-effect free, even
after tree-shaking every binding away, and `external` is not declared
side-effect free — so the built client opened with six bare `require`s it
never used: `edge-core-js`, `biggystring`,
`csv-stringify/lib/browser/sync`, `sha.js`, `sprintf-js` and `date-fns`.
`src/cli/index.ts` states the opposite as its contract ("Nothing here
imports `edge-core-js`. The engine owns core; this half owns argv, the
socket and the output"), and `docs/EDGE_CLI.md` presents one process per
command as the normal mode, so a script of twenty commands paid it twenty
times.
Measured on this machine, five runs each: `node lib/edgeCli.js help` was
162 ms and is now 60 ms. All six requires are gone from the client, the
engine still requires every plugin package it genuinely loads, and both
offline suites pass against the rebuilt bundles.
`moduleSideEffects` is false for externals only. The bundle's own modules
keep theirs, because `import './bootNodeLocale'` and
`import './commands/all'` are side-effect imports and dropping them would
unregister every command.
The manifest generator now cross-checks itself against the built bundles
when they exist. It reports `date-fns` as declared but unrequired — 25 MB,
reached through `src/locales/intl.ts`, whose `format` binding no CLI path
calls, so rollup shakes it out. It stays declared on purpose: the graph
over-approximates in the safe direction, and the moment a CLI path does
call it, a missing declaration is an `npm install` that succeeds and a CLI
that dies on first run. The reverse direction — a bundle requiring
something undeclared — now fails the gate.
3.review-async.4. `addToQueue` latches `inQuery` before arming the debounce, and the only place that cleared it was `doQuery`'s terminal branch. The `.catch` around the call reported through `onQueryError` and reset nothing, so a rejection from anywhere outside `doQuery`'s per-group `try` — building the groups, or stringifying the params — left the flag latched for the life of the process. Every later arrival then took the `!inQuery` false path, armed no timer, and never settled, because `getHistoricalRate` never calls its own `reject`. In the engine that is a `get-transactions` hanging to the client's deadline, for ever, on a daemon documented as long-lived; in the GUI it is every `useHistoricalRate` row left unresolved. The sink now unlatches, settles the queue with `0` — what a rate the server cannot price already answers — and then reports, so one bad pass costs one pass. `stopRateQueue` also cleared `inQuery` without accounting for a `doQuery` already awaiting a response, so a key queued immediately after it armed a second chain running alongside the first. A pass now carries the epoch it started with and abandons its recursion if the queue was stopped. The settle loop is shared with the failure sink rather than written twice. No test constructs a live rejection, because none is reachable through the public API — which is why the finding is medium. The epoch is reachable: a new case stops the queue under a slow pass and asserts the caller settles and the module still works afterwards.
3.review-performance.3. A key the server answers for but cannot price is deliberately not cached: storing `0` would answer `0` for the life of the process, long after the rates server recovered. But nothing absorbed the repeat either, and the engine does not persist `metadata.exchangeAmount`, so an asset with no feed re-paid the entire fiat fill on every listing — measured against a stub answering 200 with `rate` absent, a 1,200-transaction wallet cost 13 upstream requests and about a second on the first, second and third listing alike, with the cache staying empty. Reachable on a hand-added custom token, a long-tail token the server has no feed for, and dates predating an asset's market, none of them rare on an old wallet. Those keys now go in a short-lived negative cache instead, bounded and cleared the same way as the rate cache. Five minutes is far longer than a listing loop and far shorter than an outage, so a repeated listing is free and a recovered server is still picked up. `engine-status` publishes `rateUnpricedCount` beside `rateCachedCount`, so an asset with no feed is visible rather than silent. The test that asserted a zero is never cached now asserts what actually matters: the zero never enters the rate cache, the repeat costs no request, the TTL is minutes rather than hours, and clearing the cache forgets it.
3.review-servers.5 and 3.review-state.9. The in-band error path never touched the logger. Anything `mapCoreError` does not recognise becomes `500 INTERNAL_ERROR` carrying only `error.message`, and `handleRequest`'s catch sent that and returned — no stack, no route, no method. `server.ts` logged only what escaped `handleRequest` itself, and `route.ts` logs response-shape drift, so the daemon recorded a documentation bug but not an actual exception. For a detached process whose one diagnostic surface is `~/.edge-cli/logs/engine-<profile>.log`, a plugin or core fault was unreproducible afterwards: the operator had the line the client printed, and the client is often a script that discarded it. A 5xx now logs the route, the code and the stack; a 4xx logs a line without one, since it is the caller's doing. The response body is unchanged. `--tcp-host=[::1]` was accepted and then answered nothing. `allowedHostnamesFor` seeded its set from the raw string, so the set held `[::1]`, while `hostnameOf` strips the brackets `URL` keeps — a caller's `Host: [::1]:9008` arrived as `::1`, missed the set, and every request was refused 403 with a message about DNS rebinding. `server.listen(port, '[::1]')` would not have bound it as an address either. The unbracketed form worked, which is what made the bracketed one a trap rather than an obvious failure. The validator moves to `tcpPort.ts`, beside the port one that exists for the same "one spelling, both entries" reason, and canonicalises once. That also makes it testable: `index.ts` runs `main()` at import, so the old private function could not be reached from a test. The new suite pins the pair that disagreed — whatever is bound must be in the allowed set under the name a real `Host` header reduces to, which for IPv6 is always bracketed.
3.review-tests.2 and 3.review-tests.3.
Seven export checks never opened a file. `ok()` asserts only `status === 0`
and the absence of an error body, and the client returns early when
`result.files == null`, so an engine that stopped assembling `files`, or a
`writeExportFiles` whose write were deleted, would have kept all seven
green. Four checks now read what was written: that `csv,qbo` produces
exactly `tx.csv` and `tx.qbo` and no extensionless `tx` — the only coverage
`exportFilePath`'s multi-format stem logic can get — that the QBO carries
`OFXHEADER:100` and is not trivially short, and that an empty wallet
exports an empty CSV.
That last one is asserted rather than fixed, deliberately. The CSV exporter
ends in `csvStringify(items, { header: true })`, and csv-stringify takes
its header from the first record's keys — so no records means no header and
a zero-byte file. It is the shared exporter the GUI uses, so emitting a
header row for an empty range would change the app's output and needs
explicit columns to do at all. Worth a decision, not a drive-by change.
The `details` half of the error-contract table could not fail.
`toHaveProperty(key)` passes for a key whose value is `undefined`, and
every arm builds `details` as a fixed object literal, so the key is always
there — the table held for a projection gutted to
`{ challengeId: undefined, challengeUri: undefined }`, which is the one
thing it existed to catch. It now carries the values core supplied and
asserts them with `toMatchObject`. `OTP_REQUIRED` listed three of its seven
fields and omitted `voucherAuth`, the second credential whose redaction
`redaction.test.ts` exists to pin, so the two halves of that guard never
met; the error is now constructed with all seven populated. Two swap-limit
cases listed one field each and in fact publish three.
`redaction.test.ts` reads its field list back out of `toErrorBody` instead
of copying it, so renaming a field in the arm fails one test or the other
rather than leaving both asserting a key nothing emits.
A successful export of nothing, not a failure, so it is published rather
than changed. The CSV exporter ends in
`csvStringify(items, { header: true })`, which takes its column names from
the first record — with no records there is no header to write, so the file
is zero bytes. QBO's envelope does not depend on the records, so
`--export-format=csv,qbo` over an empty range writes one empty file and one
with only an envelope.
Worth knowing for anyone who changes this: the GUI reaches the opposite
conclusion from the same empty string. `TransactionsExportScene` tests for
`''` and shows "nothing to export" rather than writing a file, so the
shared exporter has to keep returning it — a header row added there would
break that guard. Any change in behaviour belongs in the engine route.
3.review-repo.1. The five documentation gates run in Travis and cannot fail there. `install:` is `npm ci` then `npm run prepare`, and `prepare.sh` calls `npm run docs:api` in write mode; `script:` then runs the gates, whose first check only asks whether regenerating changes anything — which it cannot, having just been run. Reproduced the reviewer's test: editing one JSDoc summary makes all five artifacts stale, the gates fail locally, and after a `docs:api` they all pass and exit 0. So a route edit committed without regenerating passes CI, and the comment in `.travis.yml` stated the premise it then defeated. The husky hook was the only live check, and `--no-verify` skips it. `docs:api:committed` asks git rather than the generators: prepare skips a write when nothing changed, so a dirty path under `src/cli/generated` or `docs/api/dist` *is* the staleness. It runs after the gates, where prepare has already done the regenerating, and names the stale file. Verified both ways — it passes on a clean tree and fails on the same JSDoc edit that the five gates wave through. `docs/api/README.md` said the gates were "the five checks Travis and precommit run", which was true of the invocation and false of the enforcement. It now says what each one answers and why CI needs both.
3.review-repo.2.
`jsonSchema` split a resolved type on every `|`, with no idea of nesting, and
its enum and literal branches tested for a quote character the TypeScript
checker does not emit. The committed OpenAPI document therefore carried 24
`anyOf` nodes whose every member was `{description}` — always-true in JSON
Schema 2020-12, so those fields validated anything — 15 of them built from
halves of a type cut mid-brace, and the word `enum` appeared nowhere in 117
paths. `{ [keys: string]: string | string[]; }` came apart into
`{ [keys: string]: string` and `string[]; }`; `createWallets.items` into
three fragments.
Unions now split on top-level `|` only, tracking brace, bracket, angle and
paren depth and skipping quoted text, with a guard for the unmatched `>` in
a function type. Both quote styles count as literals, so the enum branch
fires and a single literal publishes `const`. An index signature becomes
`additionalProperties`, an object literal becomes real `properties` with
`required`, and a shape this cannot read still says `type: object` rather
than falling through to a schema that accepts anything.
Measured on the regenerated document: vacuous `anyOf` nodes 24 to 1, the
cut-apart ones 15 to 0, `enum` 0 to 13. The one that remains is a union of
named core interfaces, which this generator cannot resolve to fields.
One thing the fix surfaced, not filed: the checker spells an imported type
with an absolute path, so the committed artifacts held 31 occurrences of
whichever home directory last regenerated them — a local path published in
a document, and the reason `docs:api:committed` could never have passed for
a second developer. `stableTypeText` strips the `import("…")` wrapper, and
all three artifacts are now free of it.
Eight documentation statements this branch publishes were wrong about the code beside them. The published ones are what `edge-cli help` and the OpenAPI reference print, so each was a contract a caller could read and act on. `idleShutdownAt` claimed null while "a request being served" held the engine open. It reads `heldByClients`, which excludes in-flight requests on purpose — `GET /engine/status` is itself in flight — so the documented value was not merely absent but unreachable. The field now states the three holds it reports and says why the fourth is not among them, and the route's `@returns`, which published a third and different list as the 200-response prose, points at the fields. Two handle kinds belong to no session, not one: `pendingLogin` and the `lobby` that `admin-make-lobby` parks for its whole five-minute TTL. Both hold the engine open and only the first was named, in the field prose, the guide and `sessionlessCount`'s own docblock. `transportAuth.ts` justified itself with a leak the branch closed on purpose: `GET /engine/sessions` truncates every id it lists. A reader who trusted that paragraph would either hunt a live leak or read it as licence to un-truncate the listing. The guards are right for the reason they now give. The guide's exit-code table said `1` covered every unmapped failure. Three rules sit ahead of it — an unmapped code with HTTP 503 exits 6, an unreachable engine 7, bad argv 2 — and the generated reference already stated them. `test:cli:node-safe` was credited with loading every CLI module. It loads 22 hand-listed shared modules and the client entry; the engine graph is covered by `test:cli:offline`, which spawns the real engine. The module map it was also credited with keeping honest was missing two entries, so `docs:api:verify` now fails on a module that is not in it — proven by deleting one. `EXAMPLE_TCP_PORT` claimed to be the one declaration the help text cites. `flagTable.ts` carries no numbers, so `--help` cites none; the real readers are three usage errors and two OpenAPI strings. Six JSDoc blocks sat directly above another block, documenting nothing, two of them at sites a previous pass had already fixed by hand. They are back on their symbols, and a test scans for the pattern so the next edit cannot reintroduce it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four strings the branch publishes were written twice, or written against prose nothing produces. `normalizePosixLocale` tested for the C locale before stripping the encoding suffix, so `LANG=C.UTF-8` — the Debian-family container and CI default, which is where a scripted `edge-cli` usually runs — came out as the tag `C`. That has no translation table, so every engine start warned "No translation table for locale C" about a locale that is English by definition, `engine-status` published `locale: "C"` under a field documented as a language tag, and `C` was the one value that reached `Intl.NumberFormat` and threw. The test covered only the unsuffixed spellings, which is why it survived; it now covers the spellings an environment actually sets. `isMissingFile` decided control flow by regex over a library's English, and one of its three alternatives matched a message nothing in the tree emits. Disklet rejects a missing file with `Cannot load "<file>"` from every in-memory backend, and with an `ENOENT` errno from the node one; `File not found` appears in no JavaScript under `node_modules`, and it was the sentinel every unit stub threw — so the suites exercised the one arm production cannot reach, and that arm was the broad one. Both callers answer an absent file by rewriting it from defaults, so reading an unrelated "not found" as absent is data loss. The stub now raises what disklet raises, there is a second stub for the errno, and the match set is the two spellings that exist. Fifteen commands carried a hand-written one-line description that nothing rendered: `help.ts` reads `docs?.summary ?? target.help`, and every one of the fifteen has a route with a summary. Each was worded differently from the line that ships, so fixing a wording bug in `wallet.ts` changed nothing a user saw. They are gone, `help` — the one command with no route — keeps its string, and `docs:api:verify` now fails on a `help:` beside a published summary. Two of `fieldDocs.ts`'s constants had no importer, in the module whose whole purpose is to be the one place a repeated field is described — and `WALLET_ID_DOC` stated the field differently from the description really published. They are wired up, along with two more handle strings that were written out four and three times and a TTL stated three ways, and `docs:api:verify` fails on a `_DOC` nothing reads. Every byte of `openapi.json`, `index.html` and `helpDocs.json` is unchanged by that de-duplication, which is the point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four claims about how this repository is built were wrong. The signed CLI had no documented way to build it. An earlier change moved the native signer out of `build:cli` into `build:cli:all`, `build:cli:native` and `build:cli:copy-native`, and no document followed: those names appeared in `package.json` and nowhere else, while the guide still presented `build:cli` as the built artifact and told the reader the engine prefers the native signer when available. `HMAC_SIGNING.md` was worse than silent — its authoritative list of generated outputs named two pairs where there are now three, its list of native modules two where there are now three, and its runtime-pad paragraph one bundle id where `makeApiSigner.ts` now bakes the Node one in too. Both documents now say what exists, and the guide states the `edgeKey.json` prerequisite next to the signer paragraph. CI exercised the sources and never the bundle. `test:cli:offline:built` reaches `verify` through `test:all`, but Travis stopped at `test:cli:offline`, which runs through `sucrase/register`. Three transforms produce a working CLI, the guide already explains that a defect can exist in only one of them, and both severity-0 findings the preceding QA loop turned up existed in the bundle alone — so the one artifact users are told to run was gated by a command a human types. `build:cli` is a two-second rollup; it is in the `script` list now. `node-gyp@^13` cannot run on any Node this repository claims to support. Its own `engines` is `^22.22.2 || ^24.15.0 || >=26.0.0` against `engines.node: ">=18"` and Travis's `node_js: 18`, and it is the only top-level `node-gyp` in the tree, so the range was this branch's choice. Nothing breaks today, because there is no `engine-strict` and no CI job compiles the addon — but `build:cli:all`, the only way to produce the signer, was unsupported on every Node the repository admits. `^11.5.0` is `^18.17.0 || >=20.5.0`. It costs about 37 dev-only transitive packages, since node-gyp 11 bundles `make-fetch-happen` where 13 uses `undici`; the alternative is raising the repository's own floor, which is a wider decision than this branch should take. Verified by deleting `native/edge-api-signer/node/build` and rebuilding: the addon compiles and `test:cli:node-hmac` passes all three checks. `precommit` grew about two and a half minutes for every commit in the repository, including the great majority that cannot touch `src/cli` — measured here as 55 s of documentation gates, 4.6 s of the node-safe smoke and 90.6 s of the offline suites. The cost is not the wait but that a hook this slow is the one people skip with `--no-verify`, which also skips the `tsc` and `jest` that predate the CLI. Those three steps now run only when the commit stages something under `src/cli`, `scripts/`, `docs/api` or `docs/EDGE_CLI.md`: 0.13 s otherwise, unchanged when it matters, and CI runs them regardless. Also gone: `scripts/testCliInteractive.ts` and the `test:cli:interactive` and `test:cli` scripts. The first is named for the interactive prompt and never touches it — it makes four one-shot `execSync` calls — and real prompt coverage now lives in `testCliFake.ts`; the second two were referenced by nothing, and `test:cli` spelled out a subset of `test:cli:network` a third way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five defects where the engine or the client discarded something, and the tests that would have caught each one. A pending edge login left a logged-in account nobody could name. The watcher creates the session the moment the phone approves, whether or not anyone is polling — that is what makes `--no-wait` work — so the ordinary sequence of request, approve at t=10s, nothing polls, handle expires at t=300s left an `EdgeAccount` open under an id no process had learned. `engine-sessions` truncates it, so `logout` could not name it; the only ways out were the hour-long auto-logout or `engine-stop`, and with `autoLogoutTimeInSeconds: 0` neither is bounded. It also held the engine alive indefinitely, being the first hold in `heldByClients`. Expiry now logs out a session nobody was told about, and leaves alone one a poll has already reported. The client cleared a `session.json` holding a different session. The guide publishes `--session` and `EDGE_CLI_SESSION` as an override of the persisted id, and three paths reached `setSessionId(null)` with an id the file never held: a 401 after a stale override, `logout`, and `delete-remote-account`. So `edge-cli --session=<stale> currency-wallets` answered 401 and deleted the only copy of a live session's id. The file is now touched only when the id being forgotten is the one on disk, re-read rather than snapshotted. The offline check is blunt about the cost: without the fix 124 of 179 checks fail, because one 401 wipes the file and every later command reports "Please log in first". A `?walletId=` stream silently dropped `session.created` and `session.expired` — the two account events a session-scoped stream exists to carry — in exchange for narrowing a set of wallet-scoped events that is still empty, since no emitter produces that scope. A narrower filter must not lose what a broader one would deliver. The full 3x3 scope matrix is tested now; the wallet row was the one never run, which is why this survived. The engine log's roll had three defects, all of which the injectable ceiling exposed: with 8 MB as the only way in, none of this could be reached from a test. It opened the file lazily, so a roll in that window renamed a file that did not exist yet and the pending open re-created it — the lines leading up to the roll landed in the new generation instead of the one they filled. It ended the old stream before learning whether the rename had succeeded, so a failed rename left two streams appending to one path, out of order. And nothing awaited the ended stream, so a shutdown just after a roll could lose the tail of `.1`. The log is opened with `openSync` now, renamed before the stream is touched, and `close` waits for both generations. `rates-query` and `rates-usd-to-native` declared `date` as a string and passed it to `new Date(...)`, so an unparseable date produced `isoDate: null` upstream and a rate key nothing could match — and came back inside a 200 as `rate: 0`, indistinguishable from a rate the server cannot supply, or as `404 No USD rate for bitcoin/null`, which blames the asset. They take `asQueryDate` now, the same cleaner `get-transactions` uses, so a bad date is the 400 they publish. `rates-usd-to-native` is the only route whose output is a spend amount. Along the way: `--flag ""` is a usage error in both parsers rather than one; `object-get` declares the `value` it has always returned, so the published schema no longer omits the field the route exists for; `checkCoreAlignment` fails when it resolves nothing rather than printing a tick for zero routes; the body-kind check `asBodyNumber`'s docblock has claimed since it was written now exists; `SessionListing` is branded, so the compiler really does refuse what the comment said it refused; the dead `bodyFlag` pipeline and its six consumers are gone, and `preset` is typed as the booleans the pipeline can carry; and offline coverage is 117 of 118 commands, because the rate and swap validation arms never needed the network that excused them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`testCliFake.ts` runs with `EDGE_CLI_CHECK_RESPONSES=strict`, and its own header advertises strict checking as a property of "the offline suites". `testCliSubscribe.ts` set nothing, so it ran in the engine's default `warn` mode: every response it provokes — `/engine/status` over both the unix socket and TCP, and every SSE frame — was compared against no `returns` cleaner, and a shape that had drifted from the published reference would have been logged into the engine's own log file where nothing reads it. It is set once on this process rather than per spawn, because all six `spawn`/`spawnSync` calls there inherit the environment, and so does the engine the client spawns. The two decisions `route.ts` makes about a request and a response had no test. `queryToObject` is the single layer that decides `?x=` means absent rather than `''` — three downstream branches once assumed otherwise and none of them could fire — and it is unreachable from the CLI, since `commandArgs.ts` refuses `--flag=` before a request exists, so only a raw REST call reaches it and the suites make three. `checkResponse`'s default mode is the one nothing ran: `strict` is what the suites set, and `warn` — log the drift, send the body through, which is what production does — and `off` were asserted nowhere. Both are exported for their tests, and the guide now states the two modes and why the engine's default is the lenient one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
edge-cli/ engine).develop.testModeconfig, etc.).Notes for reviewers
develop(config/keys, native HMAC, Node-safe splits, CLI). It is intentionally draft-style until dependencies / base strategy are finalized; there is nofuture!pseudo-merge in the history.publish:cli) is a placeholder until packaging/bin metadata is restored.edgeKey.json(build:cli:native); stub builds are refused.Test plan
npm run test:cli:node-safenpm run build:cli/npm run build:cli:native(withedgeKey.json)npm run test:cli:node-hmac(withedgeKey.json)npm run test:cli/npm run test:cli:edge-loginas applicablenpm test/tsc